-
Notifications
You must be signed in to change notification settings - Fork 255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Solves issue #607 #608
Solves issue #607 #608
Conversation
@@ -55,7 +56,7 @@ public function initialize(array $config): void | |||
$schema = $this->_table->getSchema(); | |||
/** @var string $field */ | |||
foreach (array_keys($this->getConfig()) as $field) { | |||
$schema->setColumnType($field, 'upload.file'); | |||
$schema->addColumn($field, ['type' => 'upload.file']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A much simpler solution is just wrapping the setColumnType()
call within a hasColumn()
check, which won't require any additional changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw your suggestion, I'll try that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fact, I don't think so. Those fields are "virtual". They don't reside in the table. hasColum will always return false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would and hence nothing needs to be done. In 5.0 the setColumnType()
call simply returned without doing anything if the column did not actually exist in the schema. So things remain the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 5.0, it would add the non-existant column in the schema, or I did something wrong
object(Cake\Database\Schema\TableSchema) id:0 {
'table' => 'attachments'
'columns' => [
'id' => [ ],
'name' => [ ],
'size' => [ ],
'type' => [ ],
'dir' => [ ],
'created' => [ ],
'modified' => [ ],
'email_heading' => [ ],
'thumbnail' => [ ],
'poster' => [ ],
'illustration' => [ ],
]
email_heading, thumbnail, poster and illustration here above are the attachments defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 5.0, it would add the non-existant column in the schema...
No it doesn't https://github.com/cakephp/cakephp/blob/5.0.11/src/Database/Schema/TableSchema.php#L379-L383
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, sorry, I didn't clear the cache after having downgraded to 5.0.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #608 +/- ##
============================================
- Coverage 95.84% 95.80% -0.05%
- Complexity 109 110 +1
============================================
Files 11 11
Lines 289 286 -3
============================================
- Hits 277 274 -3
Misses 12 12 ☔ View full report in Codecov by Sentry. |
@@ -71,7 +71,7 @@ public function testInitialize() | |||
->onlyMethods(['setColumnType']) | |||
->disableOriginalConstructor() | |||
->getMock(); | |||
$schema->expects($this->once()) | |||
$schema->expects($this->any()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing this add the field
column to the schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean adding the field
column in tests/schema.php
? Doing this does nothing by itself, and test are failing:
- Josegonzalez\Upload\Test\TestCase\Model\Behavior\UploadBehaviorTest::testInitialize
Expectation failed for method name is "setColumnType" when invoked 1 time.
Method was expected to be called 1 time, actually called 0 times.
What I understand is that, as the schema is a mock, I have to mock the 'hasColumn' method as well, and return true.
$schema = $this->getMockBuilder('Cake\Database\Schema\TableSchema')
->onlyMethods(['setColumnType', 'hasColumn'])
->disableOriginalConstructor()
->getMock();
$schema->expects($this->once())
->method('hasColumn')
->with('field')
->willReturn(true);
The test then passes, but then I don't see the point of modifying the schema in that case, or is there something else that I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you'll need to add expects for hasColum()
too.
We need to separately test for the two cases where the column is in the schema and when it's not and not use any()
as it's less work :)
Solves problem induced by cake v5.1